WIP: OCPBUGS-61756: ctrl: kubeletConfig: avoid updating ConfigMap on Paused MCP by shajmakh · Pull Request #2261 · openshift-kni/numaresources-operator

shajmakh · 2025-10-09T12:23:08Z

Having a paused MCP should prevent updating/creating the corresponding
config map for the specified node group. So far, the code wasn't
considering the case of paused MCPs, which leads to creating/updating the config map
a thing that caused a mismatch between the configuration in the config
map and the one reflected on the NRTs.
In this commit, we modify the kubeletconfig controller to skip on
updating RTE config maps that belong to paused MCPs.

openshift-ci · 2025-10-09T12:23:23Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: shajmakh

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details

Needs approval from an approver in each of these files:

~~OWNERS~~ [shajmakh]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

shajmakh · 2025-10-14T09:30:43Z

/hold

Tal-or

Good change overall.

In case the MCP is paused, the final update to the configmap will be triggered when the MCP changes back to unpause?

shajmakh · 2025-11-04T16:17:21Z

/retest

shajmakh · 2025-11-04T16:21:01Z

relies on #2324

shajmakh · 2025-11-05T07:43:44Z

depends on #2426

openshift-ci-robot · 2025-11-05T12:10:14Z

@shajmakh: This pull request references Jira Issue OCPBUGS-61756, which is invalid:

expected the bug to target the "4.21.0" version, but no target version was set

Comment /jira refresh to re-evaluate validity if changes to the Jira bug are made, or edit the title of this pull request to link to a different bug.

The bug has been updated to refer to the pull request using the external bug tracker.

Details

In response to this:

Having a paused MCP should prevent updating/creating the corresponding
config map for the specified node group. So far, the code wasn't
considering the case of paused MCPs, which leads to creating/updating the config map
a thing that caused a mismatch between the configuration in the config
map and the one reflected on the NRTs.
In this commit, we modify the kubeletconfig controller to skip on
updating RTE config maps that belong to paused MCPs.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the openshift-eng/jira-lifecycle-plugin repository.

shajmakh · 2025-11-06T07:51:23Z


-func (em *ExistingManifests) MachineConfigsState(mf Manifests) ([]objectstate.ObjectState, MCPWaitForUpdatedFunc) {
+func (em *ExistingManifests) MachineConfigsState(mf Manifests) ([]objectstate.ObjectState, MCPWaitForUpdatedFunc, sets.Set[string]) {
+	pausedMCPs := sets.New[string]()


we may want to call this "mcpsToSkip" to include cases where the MCP is empty (solves https://issues.redhat.com/browse/OCPBUGS-52859)

tested locally and it made no difference to include empty MCPs, besides the bug no longer reproduces.
I don't tend to include the empty MCPs in this patch given this data, but if this bug is to hit again, the fix should be straightforward to include the zero-machine-count MCPs as followes:

// those are either paused MCPs or empty MCPs (no machine count) ignoredMCPs := sets.New[string]() var ret []objectstate.ObjectState if mf.Core.MachineConfig == nil { return ret, nullMachineConfigPoolUpdated, ignoredMCPs } enabledMCCount := 0 for _, tree := range em.trees { for _, mcp := range tree.MachineConfigPools { if mcp.Spec.Paused || mcp.Status.MachineCount == 0 { klog.Warningf("the machine config pool %q is paused or have zero machine count", mcp.Name) ignoredMCPs.Insert(mcp.Name) continue }

yanirq · 2026-01-06T10:49:21Z

/retest

yanirq · 2026-01-06T10:50:42Z

also fixes https://issues.redhat.com/browse/OCPBUGS-52859

shajmakh · 2026-01-15T13:21:54Z

/unhold

ffromani

initial review

shajmakh · 2026-01-23T15:27:51Z

/retest

This is a preliminary step to expand the scenarios where the reconciliation can return more possible errors where some are tolerable. Signed-off-by: Shereen Haj <shajmakh@redhat.com>

Having a paused MCP should prevent updating the corresponding config map for the specified node group. So far, the code wasn't considering the case of paused MCPs, which lead to creating/updating the config map to the newest kubeletconfig CR updates,a thing that caused a mismatch between the configuration in the config map vs the one reflected on the NRTs. In this commit, we modify the kubeletconfig controller to handle paused MCPs such that it skips updating existing RTE config maps; and for new node groups whose MCP is paused, the controller will fetch the old machineConfig (before the pause) and creates RTE config map based on the decoded kubeletconfig data from it. Signed-off-by: Shereen Haj <shajmakh@redhat.com>

We want to know when a paused MCP goes unpaused and do the needed updates to the config map when that happens. In previous commit, when a paused MCP was detected, the reconcile would requeue after some time; that was intended to catch the unpause operation eventually. In this commit, we don't requeue on detected paused MCP but rather watch for pause field updates on targeted MCPs. Signed-off-by: Shereen Haj <shajmakh@redhat.com>

The situtaion is mainly mitigated in the kubeletconfig controller, however we have a bug in the NRO controller such that it puts the NRO CR in progressing state because paused MCP is not in updated condition. This commit ignores checking whether a paused MCP is up-to-date, and introduces a new status condition to report paused MCPs if exist. Signed-off-by: Shereen Haj <shajmakh@redhat.com>

Node group can be set with either MCPselector or a poolName. Validation of the nodegroup configuraion is checked in the reconcile logic, and because poolName is set and MCPSelector is not a valid nodeGroup we need to consider this and reconcile. This was missed when nodeGroup.PoolName was first introduced and didn't cause any observed reconcile failures because: 1. any change on the nodeGroups would anyway trigger reconciliation 2. so far the MCP related updates were caused by Kubeletconfig updates which wasn't blocked until the paused-MCP case got handled (which is why this appeared now). Signed-off-by: Shereen Haj <shajmakh@redhat.com>

shajmakh · 2026-01-26T10:02:01Z

/retest

shajmakh · 2026-02-18T11:28:41Z

/retest

openshift-ci · 2026-02-18T13:01:00Z

@shajmakh: The following test failed, say /retest to rerun all failed tests or /retest-required to rerun all mandatory failed tests:

Test name	Commit	Details	Required	Rerun command
ci/prow/ci-e2e-install-hypershift	`22533cb`	link	true	`/test ci-e2e-install-hypershift`

Full PR test history. Your PR dashboard.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. I understand the commands that are listed here.

shajmakh · 2026-03-04T08:03:27Z

Good change overall.

In case the MCP is paused, the final update to the configmap will be triggered when the MCP changes back to unpause?

I apologize for missing this question; yes that is the correct behavior, and is covered in the controller tests.

ffromani

the direction seems fine, I have implementation comments.
The last commit would need another review round from me, need more time to grasp it.

ffromani · 2026-03-04T09:04:08Z

 	setCtrlRef func(owner, controlled metav1.Object, scheme *runtime.Scheme, opts ...controllerutil.OwnerReferenceOption) error
 }

+type reconcileErrorHandler struct {


this is not a handler. a Handler manages something. This seems more a reconcileResult

can we reuse a reconcile.Step, extend it, or even create a variant of?

ffromani · 2026-03-04T09:07:03Z

 		}
+
+		if mcp.Spec.Paused {
+			klog.InfoS("detected paused MCP", "name", mcp.Name)


this will be printed at every reconciliation step it seems. Let's make it V(4)

ffromani · 2026-03-04T09:08:31Z

+					}
+				}
+
+				// use local version of github.com/openshift/machine-config-operator/pkg/controller/common.ParseAndConvertConfig


this is just narrating in english what the code does. Either give context about why or remove the comment

ffromani · 2026-03-04T09:20:30Z

+				}, reconcileErrorHandler{result: ctrl.Result{Requeue: true, RequeueAfter: MachineConfigPoolPausedRetryPeriod}}
+			}
+
+			klog.InfoS("MachineConfigPool of KubeletConfig %s is paused and configMap %s exists", kcKey.Name, existingCM.Name)


please check verbosity to avoid log spam

ffromani · 2026-03-04T09:22:04Z

+				Namespace: testNamespace,
+				Name:      objectnames.GetComponentName(nro.Name, poolName2),
+			}
+			Expect(reconciler.Client.Get(context.TODO(), key, cm)).To(HaveOccurred())


and check IsNotFound perhaps?

ffromani · 2026-03-04T09:23:02Z

+			fakeRecorder, ok := reconciler.Recorder.(*record.FakeRecorder)
+			Expect(ok).To(BeTrue())
+			event := <-fakeRecorder.Events
+			Expect(event).To(ContainSubstring("ProcessOK"))
+			Expect(event).To(ContainSubstring("Updated RTE config"))


ffromani · 2026-03-04T09:23:18Z

+			fakeRecorder, ok := reconciler.Recorder.(*record.FakeRecorder)
+			Expect(ok).To(BeTrue())
+			event := <-fakeRecorder.Events
+			Expect(event).To(ContainSubstring("ProcessOK"))
+			Expect(event).To(ContainSubstring(mcoKCPaused.Name))


ffromani · 2026-03-04T09:24:48Z

 	return requests
 }

+func (r *KubeletConfigReconciler) nodeGroupToMachineConfigPool(ctx context.Context, object client.Object) []reconcile.Request {


do we use the object argument?

ffromani · 2026-03-04T09:26:07Z

+
+	ngstatus := nro.Status.NodeGroups
+	targetMCPs := sets.New[string]()
+	for _, ngstatus := range ngstatus {


please don't use a loop variable named like the collection it iterates on

that I missed. Thanks for catching.

ffromani · 2026-03-04T09:28:55Z

+		klog.ErrorS(err, "failed to list KubeletConfigs %v")
+	}
+	//map mcp to kubeletconfig requests
+	for _, mcpName := range targetMCPs.UnsortedList() {


can't we just do

for _, ngstatus := range nro.Status.NodeGroups { mcpObj, ok := mcpMap[ngstatus.PoolName] if !ok { continue } mcpLabels := labels.Set(mcpObj.Labels) ...

Tal-or

Some comments inside.
One thing that bother me the most is the tolerateError concept.
if it's tolerable, I wouldn't consider it an error.

can't we simply log the error in place, and Requeue the request as we usually do?

Tal-or · 2026-03-04T09:19:43Z


 	r.Recorder.Event(instance, "Normal", "ProcessOK", fmt.Sprintf("Updated RTE config %s/%s from kubelet config %s", cm.Namespace, cm.Name, req.NamespacedName.String()))
-	return ctrl.Result{}, nil
+	//return ctrl.Result{}, nil


leftovers I presume

Tal-or · 2026-03-04T09:21:24Z

+	if err != nil {
+		return nil, reconcileErrorHandler{err: err}
+	}
+	return cm, errHandler // FIXME use predicate


Is there a reason why not to implement predicate in this PR already? it's not suppose to be lot of code, so I guess we can squeeze it here as another small commit.

Tal-or · 2026-03-04T10:26:28Z

+				if !apierrors.IsNotFound(err) {
+					return nil, reconcileErrorHandler{
+						err:           err,
+						tolerateError: true,


If we need to support the operation even when MCP are paused, why these errors are tolerate?

shajmakh · 2026-03-04T14:42:06Z

Some comments inside. One thing that bother me the most is the tolerateError concept. if it's tolerable, I wouldn't consider it an error.

can't we simply log the error in place, and Requeue the request as we usually do?

Probably I picked the wrong name for the struct and the fields inside. Introducing the new struct was meant to hold the reconciliation result and interpret it as an event which can be successful complete reconciliation, failure, or skipped reconciliation due to an unblocking error.
Before introducing the second commit, the only error the reconciliation would be skipped on was an unfound kubeletconfig. Post introducing the paused MCP logic, we have other produced errors in the flow that are tolerable; hence we need a way to distinguish between a legit error that will cause reconcile to fail and between errors that may occur on reconcile but are not worth to mark the iteration as failed.
I'll visit this logic again to see if there is anything we can do to make it simpler to grasp or reuse what already exists.

openshift-ci · 2026-04-30T20:34:38Z

PR needs rebase.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository.

openshift-ci Bot requested review from Tal-or and swatisehgal October 9, 2025 12:23

shajmakh changed the title ~~ctrl: kubeletConfig: avoid updating ConfigMap on Paused MCP~~ WIP: ctrl: kubeletConfig: avoid updating ConfigMap on Paused MCP Oct 9, 2025

openshift-ci Bot added approved Indicates a PR has been approved by an approver from all required OWNERS files. do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels Oct 9, 2025

shajmakh force-pushed the kubeletconfig-fix branch from 92eaeed to 80910ba Compare October 13, 2025 09:08

shajmakh changed the title ~~WIP: ctrl: kubeletConfig: avoid updating ConfigMap on Paused MCP~~ ctrl: kubeletConfig: avoid updating ConfigMap on Paused MCP Oct 13, 2025

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Oct 13, 2025

shajmakh force-pushed the kubeletconfig-fix branch from 80910ba to 29846b9 Compare October 13, 2025 09:12

openshift-ci Bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Oct 14, 2025

Tal-or reviewed Oct 15, 2025

View reviewed changes

Comment thread internal/reconcile/event.go Outdated

Comment thread internal/reconcile/step.go Outdated

shajmakh force-pushed the kubeletconfig-fix branch from 29846b9 to 1e86d9a Compare November 4, 2025 16:14

shajmakh changed the title ~~ctrl: kubeletConfig: avoid updating ConfigMap on Paused MCP~~ WIP: ctrl: kubeletConfig: avoid updating ConfigMap on Paused MCP Nov 4, 2025

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 4, 2025

shajmakh force-pushed the kubeletconfig-fix branch 4 times, most recently from b413287 to e133797 Compare November 5, 2025 12:08

shajmakh changed the title ~~WIP: ctrl: kubeletConfig: avoid updating ConfigMap on Paused MCP~~ OCPBUGS-61756: ctrl: kubeletConfig: avoid updating ConfigMap on Paused MCP Nov 5, 2025

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Nov 5, 2025

shajmakh commented Nov 6, 2025

View reviewed changes

shajmakh force-pushed the kubeletconfig-fix branch from e133797 to 1b503cf Compare November 27, 2025 10:31

shajmakh force-pushed the kubeletconfig-fix branch 2 times, most recently from e25bcee to f5ad21e Compare January 15, 2026 13:12

openshift-ci Bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 15, 2026

ffromani reviewed Jan 16, 2026

View reviewed changes

Comment thread internal/reconcile/step.go Outdated

Comment thread internal/reconcile/step.go Outdated

Comment thread internal/reconcile/step.go Outdated

Comment thread internal/kubeletconfig/kubeletconfig.go

Comment thread internal/controller/kubeletconfig_controller.go Outdated

Tal-or reviewed Jan 18, 2026

View reviewed changes

Comment thread internal/controller/kubeletconfig_controller.go Outdated

Comment thread internal/controller/kubeletconfig_controller.go

Comment thread internal/controller/kubeletconfig_controller.go Outdated

shajmakh force-pushed the kubeletconfig-fix branch from f5ad21e to 5e477f1 Compare January 23, 2026 13:54

shajmakh changed the title ~~OCPBUGS-61756: ctrl: kubeletConfig: avoid updating ConfigMap on Paused MCP~~ WIP: OCPBUGS-61756: ctrl: kubeletConfig: avoid updating ConfigMap on Paused MCP Jan 23, 2026

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 23, 2026

shajmakh force-pushed the kubeletconfig-fix branch 2 times, most recently from c129242 to 22533cb Compare January 23, 2026 15:25

shajmakh changed the title ~~WIP: OCPBUGS-61756: ctrl: kubeletConfig: avoid updating ConfigMap on Paused MCP~~ OCPBUGS-61756: ctrl: kubeletConfig: avoid updating ConfigMap on Paused MCP Jan 23, 2026

openshift-ci Bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 23, 2026

shajmakh added 5 commits January 23, 2026 17:29

ctrl: kubeletconfig: introduce reconcileErrorHandler

1520766

This is a preliminary step to expand the scenarios where the reconciliation can return more possible errors where some are tolerable. Signed-off-by: Shereen Haj <shajmakh@redhat.com>

ffromani reviewed Mar 4, 2026

View reviewed changes

Tal-or reviewed Mar 4, 2026

View reviewed changes

shajmakh changed the title ~~OCPBUGS-61756: ctrl: kubeletConfig: avoid updating ConfigMap on Paused MCP~~ WIP: OCPBUGS-61756: ctrl: kubeletConfig: avoid updating ConfigMap on Paused MCP Mar 6, 2026

openshift-ci Bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 6, 2026

openshift-ci Bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Apr 30, 2026

Conversation

shajmakh commented Oct 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

openshift-ci Bot commented Oct 9, 2025

Uh oh!

shajmakh commented Oct 14, 2025

Uh oh!

Tal-or left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

shajmakh commented Nov 4, 2025

Uh oh!

shajmakh commented Nov 4, 2025

Uh oh!

shajmakh commented Nov 5, 2025

Uh oh!

openshift-ci-robot commented Nov 5, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

yanirq commented Jan 6, 2026

Uh oh!

yanirq commented Jan 6, 2026

Uh oh!

shajmakh commented Jan 15, 2026

Uh oh!

ffromani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

shajmakh commented Jan 23, 2026

Uh oh!

shajmakh commented Jan 26, 2026

Uh oh!

shajmakh commented Feb 18, 2026

Uh oh!

openshift-ci Bot commented Feb 18, 2026

Uh oh!

shajmakh commented Mar 4, 2026

Uh oh!

ffromani left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Tal-or left a comment

shajmakh commented Oct 9, 2025 •

edited

Loading